Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't show blank IPs metadata row for containers with no IP #960

Merged
merged 3 commits into from
Feb 19, 2016
Merged

Don't show blank IPs metadata row for containers with no IP #960

merged 3 commits into from
Feb 19, 2016

Conversation

paulbellamy
Copy link
Contributor

Previously we were adding the blank default IP, and showing that (an empty row), but we should skip the row if there are no IPs.

@2opremio ptal

@paulbellamy paulbellamy added this to the 0.13.0 milestone Feb 12, 2016
@2opremio
Copy link
Contributor

When does this happen? If it's due to -net containerID (or even -net host) we should try to look further instead of simply not showing it.

@2opremio 2opremio assigned paulbellamy and unassigned 2opremio Feb 15, 2016
@paulbellamy
Copy link
Contributor Author

The specific case I was seeing is when --net=host. Hmm.. so maybe we should show the host IPs instead, you're saying?

@2opremio
Copy link
Contributor

Yes, or the referenced container IPs if it also happens with -net containerId.

On Tuesday, February 16, 2016, Paul Bellamy notifications@github.com
wrote:

The specific case I was seeing is when --net=host. Hmm.. so maybe we
should show the host IPs instead, you're saying?

—
Reply to this email directly or view it on GitHub
#960 (comment).

@paulbellamy paulbellamy force-pushed the no-ips-for-no-network branch from d427e38 to 6b73da0 Compare February 17, 2016 15:44
@paulbellamy paulbellamy force-pushed the no-ips-for-no-network branch from 6b73da0 to df856d7 Compare February 17, 2016 16:42
@paulbellamy
Copy link
Contributor Author

@2opremio wdyt?

@paulbellamy paulbellamy assigned 2opremio and unassigned paulbellamy Feb 17, 2016
2opremio
2opremio reviewed Feb 17, 2016
View reviewed changes
render/topologies.go

for id, c := range containers {
networkMode, ok := c.Node.Latest.Lookup(docker.ContainerNetworkMode)
if !ok || networkMode != docker.NetworkModeHost {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

@2opremio 2opremio assigned paulbellamy and unassigned 2opremio Feb 17, 2016
@2opremio
Copy link
Contributor

Also, it would be a good idea to add a test.

@2opremio
Copy link
Contributor

Minor comment, extra issue and tests-aside LGTM

@paulbellamy paulbellamy mentioned this pull request Feb 19, 2016
Closed
@paulbellamy
Copy link
Contributor Author

Refactored a bit and added test, @2opremio wdyt?

@paulbellamy paulbellamy assigned 2opremio and unassigned paulbellamy Feb 19, 2016
@2opremio
Copy link
Contributor

LGTM, but the linter is failing: ContainerWithHostIPsRenderer should have comment or be unexported

@paulbellamy paulbellamy force-pushed the no-ips-for-no-network branch from 0580262 to 5535f5e Compare February 19, 2016 17:04
paulbellamy added a commit that referenced this pull request Feb 19, 2016
@paulbellamy
Merge pull request #960 from weaveworks/no-ips-for-no-network
d2bf204 
Don't show blank IPs metadata row for containers with no IP
@paulbellamy paulbellamy merged commit d2bf204 into master Feb 19, 2016
@paulbellamy paulbellamy deleted the no-ips-for-no-network branch February 19, 2016 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@2opremio 2opremio

Labels
None yet
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
2 participants
@paulbellamy @2opremio